Optimize column_encryption_policy checks in recv_results_rows by mykaul · Pull Request #630 · scylladb/python-driver

mykaul · 2025-12-25T17:21:24Z

There's no point in checking a global policy for every single value decoding, not for every row decoded.

Adjusted the code to only check it once per recv_results_rows() call - decode_row() should be defined either as is today with column_encryption_policy enabled,
or much simpler without all those extra checks.

Added a unit test from CoPilot.

Fixes: #582

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I have provided docstrings for the public items that I want to introduce.
I have adjusted the documentation in ./docs/source/.
I added appropriate Fixes: annotations to PR description.

mykaul · 2025-12-25T19:30:16Z

CI failure is unrelated, and I've seen it happening on too many PRs already :-/

dkropachev · 2025-12-29T13:20:57Z

tests/unit/test_protocol_decode_optimization.py

+from cassandra.protocol import ResultMessage, RESULT_KIND_ROWS
+
+
+class DecodeOptimizationTest(unittest.TestCase):


Could you please move this class to tests/unit/test_protocol.py and rename it to ResultTest

Done in 76c09ae

dkropachev · 2025-12-29T13:21:11Z

tests/unit/test_protocol_decode_optimization.py

+if __name__ == '__main__':
+    unittest.main()


Please drop it.

Done in 76c09ae

dkropachev

numpy_parser.pyx, parsing.pyx also needs to be fixed

tests/unit/test_protocol_decode_optimization.py

mykaul · 2026-01-06T08:32:40Z

numpy_parser.pyx, parsing.pyx also needs to be fixed

It wasn't clear to me what needs to fixing there:
numpy - it doesn't use/call column_encryption
parsing.pyx - there's just 'not implemented' functions there.

dkropachev · 2026-01-06T15:10:16Z

numpy_parser.pyx, parsing.pyx also needs to be fixed

It wasn't clear to me what needs to fixing there: numpy - it doesn't use/call column_encryption parsing.pyx - there's just 'not implemented' functions there.

Instead of changing only TupleRowParser class you need to do similar changes to ColumnParser and RowParser and all their implementations: ListParser, LazyParser and NumpyParser, you will discover that NumpyParser disregards column_encryption_policy which means that it does not work properly when when columns are encrypted, have a plug there that throwing NotImplementedError.
In total all Parser implementation classes should contain implementations for:

    cpdef parse_plain_rows(self, BytesIOReader reader, ParseDesc desc)

    cpdef parse_enc_rows(self, BytesIOReader reader, ParseDesc desc)

While Abstract classes:

cdef class ColumnParser:
    cpdef parse_rows(self, BytesIOReader reader, ParseDesc desc):
        if desc.column_encryption_policy:
            return self.unpack_enc_row(reader, desc)
        return self.unpack_plain_row(reader, desc)

    cpdef parse_plain_rows(self, BytesIOReader reader, ParseDesc desc)

    cpdef parse_enc_rows(self, BytesIOReader reader, ParseDesc desc)

mykaul · 2026-01-06T15:47:17Z

numpy_parser.pyx, parsing.pyx also needs to be fixed

It wasn't clear to me what needs to fixing there: numpy - it doesn't use/call column_encryption parsing.pyx - there's just 'not implemented' functions there.

Instead of changing only TupleRowParser class you need to do similar changes to ColumnParser and RowParser and all their implementations: ListParser, LazyParser and NumpyParser, you will discover that NumpyParser disregards column_encryption_policy which means that it does not work properly when when columns are encrypted, have a plug there that throwing NotImplementedError. In total all Parser implementation classes should contain implementations for:

Let me see if I understand this:

TupleRowParser - Implemented unpack_ce_row() and unpack_row()
ColumnParser - its parse_rows() creates a TupleRowParser object - and then I call its unpack_ce_row() / unpack_row()
RowParser- its unpack_row() is raising NotImplementedError - I'm not sure I should change this in this PR?
ListParser - its parse_rows() creates a TupleRowParser object - and then I call its unpack_ce_row() / unpack_row()
LazyParser - its parse_rows() calls parse_rows_lazy() - which creates a TupleRowParser object - and then I call its unpack_ce_row() / unpack_row()
NumpyParser its parse_rows() calls its _parse_rows() which completely ignores encryption and calls unpack_row() - but I have no idea where it's implemented. BUT - I'm not introducing a regression here, no?
BoundStatement.bind() - added in the last commit in the series.

Is that an accurate representation?

    cpdef parse_plain_rows(self, BytesIOReader reader, ParseDesc desc)

    cpdef parse_enc_rows(self, BytesIOReader reader, ParseDesc desc)

I'm not sure I understand the interaction between Python and Cython - I was not going to change the interfaces whatsoever.

While Abstract classes:

cdef class ColumnParser:
    cpdef parse_rows(self, BytesIOReader reader, ParseDesc desc):
        if desc.column_encryption_policy:
            return self.unpack_enc_row(reader, desc)
        return self.unpack_plain_row(reader, desc)

    cpdef parse_plain_rows(self, BytesIOReader reader, ParseDesc desc)

    cpdef parse_enc_rows(self, BytesIOReader reader, ParseDesc desc)

You raise a good point about the names of the functions - that can clearly be more clear with 'plain' and 'encrypted' in them! I'll fix that.

There's no point in checking a global policy for every single value decoding, not for every row decoded. Adjusted the code to only check it once per recv_results_rows() call - decode_row() should be defined either as is today with column_encryption_policy enabled, or much simpler without all those extra checks. Added a unit test from CoPilot. Fixes: scylladb#582 Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

…ack_row() function Very similar to the native Python code, separate the two cases, if column encryption (CE) policy is not enabled, the code is substantially simplified. If it is, it's slightly more elaborate. Decided to have two loops in two functions, one for each case, for performance reasons, even if readability-wise it's not as great. AI agreed with me: Recommendation: Keep it as is. In high-performance Cython code like this, duplicating a small block of code Fixes: scylladb#639 Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

Add tests, respond to review feedback on added tests. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

Split BoundStatement.bind() into CE and non-CE loops to avoid per-value CE checks when no policy is configured. In the CE loop, use a single uses_ce branch to select type serialization and optional encryption for each column. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

…_col_encrypted_row() Per review comments. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

mykaul · 2026-01-27T16:41:16Z

You raise a good point about the names of the functions - that can clearly be more clear with 'plain' and 'encrypted' in them! I'll fix that.

Done in latest commit. I think it's ready for re-review.

mykaul · 2026-01-27T17:03:53Z

CI failure seems like known issue #446

Instead, do it outside the loop. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

dkropachev mentioned this pull request Dec 29, 2025

Optimize column_encryption_policy checks in make_recv_results_rows #639

Open

dkropachev requested changes Dec 29, 2025

View reviewed changes

mykaul force-pushed the remove_enc_cols branch 2 times, most recently from 3a9031e to ad8b6f9 Compare January 5, 2026 08:01

dkropachev requested changes Jan 5, 2026

View reviewed changes

tests/unit/test_protocol_decode_optimization.py Outdated Show resolved Hide resolved

mykaul force-pushed the remove_enc_cols branch from 76c09ae to f44a9ba Compare January 6, 2026 08:13

mykaul force-pushed the remove_enc_cols branch from f44a9ba to 132859f Compare January 18, 2026 12:50

mykaul added 5 commits January 27, 2026 17:46

(improvement)Optimize column_encryption_policy checks: tests

bb07e9b

Add tests, respond to review feedback on added tests. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

Renamed unpack_row() -> unpack_plain_row(), unpack_ce_row() -> unpack…

d87f917

…_col_encrypted_row() Per review comments. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

mykaul force-pushed the remove_enc_cols branch from 132859f to d87f917 Compare January 27, 2026 16:29

(improvement) extract the try/except clause so it won't be per every row

4f3c748

Instead, do it outside the loop. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>

mykaul mentioned this pull request Feb 1, 2026

[RFC] (improvement)Remove CQL binary protocol v3 #683

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize column_encryption_policy checks in recv_results_rows#630

Optimize column_encryption_policy checks in recv_results_rows#630
mykaul wants to merge 6 commits intoscylladb:masterfrom
mykaul:remove_enc_cols

mykaul commented Dec 25, 2025

Uh oh!

mykaul commented Dec 25, 2025

Uh oh!

dkropachev Dec 29, 2025

Uh oh!

mykaul Jan 5, 2026

Uh oh!

dkropachev Dec 29, 2025

Uh oh!

mykaul Jan 5, 2026

Uh oh!

dkropachev left a comment

Uh oh!

Uh oh!

mykaul commented Jan 6, 2026

Uh oh!

dkropachev commented Jan 6, 2026

Uh oh!

mykaul commented Jan 6, 2026 •

edited

Loading

Uh oh!

mykaul commented Jan 27, 2026

Uh oh!

mykaul commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		from cassandra.protocol import ResultMessage, RESULT_KIND_ROWS


		class DecodeOptimizationTest(unittest.TestCase):

Conversation

mykaul commented Dec 25, 2025

Pre-review checklist

Uh oh!

mykaul commented Dec 25, 2025

Uh oh!

dkropachev Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

mykaul Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

dkropachev Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

mykaul Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

dkropachev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mykaul commented Jan 6, 2026

Uh oh!

dkropachev commented Jan 6, 2026

Uh oh!

mykaul commented Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mykaul commented Jan 27, 2026

Uh oh!

mykaul commented Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mykaul commented Jan 6, 2026 •

edited

Loading